fix(engine)!: reject engine calls made outside a template invocation - #2440

Reviewed at 0cbc852.

Verified sound

• The check sits before both the meter sync and dispatch, so no op runs and no effect commits (crates/engine/src/wasm/process.rs:181).
• No false positives: all in-invocation template code runs inside func.call, between begin_metered_invocation and end_metered_invocation. Nested cross-template calls get their own WasmProcess/WasmEnv/meter, so the callee's CallInfo alloc is correctly outside-invocation while the caller's meter stays live.
• Error precedence in alloc_checked/free_checked is right — the recorded engine error wins over MemoryAllocationFailed.
• Load-time instantiation (crates/engine/src/wasm/module.rs:136) binds stub imports, so a start-section engine call can't reach the runtime there either.
• The hardcoded 28-byte ABI blob decodes as documented (prefix includes itself, matching the existing 16-byte variant and with_memory_embedded_len).

Two things I'd change before merge.

1. The invariant is coupled to the metering lifecycle, and the stated follow-up breaks it

has_invocation_in_flight() is invocation_meter.is_some(). That is true today only because end_metered_invocation() runs before env.free(...). The "Not covered" item — "moving the accounting after the free, or opening a second metered window around it" — would make the meter live during tari_free and silently re-open exactly this hole. test_engine_call_in_tari_free would start failing, but a future variant of that refactor that only widens the window around the alloc would not be caught at all.

Make the invariant its own state (an explicit entry_context / in_template_invocation flag set around func.call), or at minimum put a load-bearing comment on begin_metered_invocation/end_metered_invocation saying the free must stay outside the window. Deriving a consensus-critical security check from a billing side-effect is the kind of thing that regresses without a compile error.

2. take_rejected_engine_call drains more than it claims

It takes the shared last_engine_error slot, which the normal dispatch error path also writes (crates/engine/src/wasm/process.rs:307-309). So on the success path, a RuntimeError recorded mid-invocation and swallowed by a template that ignored the null now surfaces out of free_checked — reported as though the free made an illegal engine call.

Two consequences:

• Misattributed diagnostics for an unrelated error.
• A second consensus-affecting change not in the Breaking section: a transaction that previously succeeded after swallowing an engine error now rejects. Templates built with tari_template_abi panic on null (crates/template_abi/src/abi/mod.rs:68) so they hit the Err path, but hand-rolled WASM is precisely the threat model here.

Surfacing it is probably the behaviour we want — but do it deliberately: drain the slot explicitly right after func.call returns Ok, and narrow take_rejected_engine_call to match only EngineCallOutsideInvocation (or give the rejection its own field).

3. Add the tari_alloc test

Flagged as uncovered in the description. It is the more dangerous half — it runs before the meter has ever been installed, and it is the first thing invoke does. A second feature variant is cheap and this is consensus code.

Follow-up (pre-existing, not introduced here)

handle's env_mut.alloc (crates/engine/src/wasm/process.rs:337) is deliberately unchecked, and correctly so — but it means a template whose tari_alloc calls tari_engine recurses host→wasm→host once per response allocation. max_call_depth: 10 does not apply (same call frame), and only EmitLog is count-capped via max_logs; something like GenerateUniqueId is not. Bounded only by metering and the native stack. Worth confirming a node cannot be stack-overflowed this way — separate issue.

Nits

• Adding #[link(wasm_import_module = "env")] and renaming debug → tari_debug changes the extern block for every buggy variant. Correct fix (the old debug matched no host import), but it is an unrelated drive-by — worth a line in the description.
• engine_call_in_free is a non-additive feature: --all-features duplicates the #[no_mangle] _ABI_TEMPLATE_DEF symbol. Same pattern as the existing return_null_abi/return_empty_abi flags, so pre-existing, but the variant count is growing.
• internal_alloc does not guard size + USIZE_SIZE overflow or a null return from alloc. Test-only; fine.

Reviewed fbd9dba and 6e29d17. Both earlier points are addressed correctly.

Addressed

• fbd9dba — in_template_invocation is now its own flag, opened and closed around func.call alone (crates/engine/src/wasm/process.rs:440-442), with a doc comment explaining why it is not derived from the meter. refused_engine_call is a separate slot carrying only EngineOp, so take_refused_engine_call can no longer drain an unrelated mid-invocation RuntimeError — the second consensus change is gone. test_engine_call_in_tari_alloc covers the alloc half; the shared execute_buggy_main helper and the call_engine() split read well.
• 6e29d17 — dropping the WasmEnv clone is semantics-preserving: Runtime is a NonNull<Box<dyn RuntimeInterface>> (crates/engine/src/runtime/mod.rs:290), so both copies always pointed at the same interface. The on_panic_handler rewrite is faithful too — the invalid-UTF8 branch returns None and records nothing, matching the old early return.

Two new findings.

1. The recursion is still open, and it is a node crash

fbd9dba's message notes that an engine call from tari_alloc "recursed host to WASM and back once per response allocation and aborted the process with a stack overflow". The guard closes that for tari_alloc driven outside an invocation. It does not close it for handle's response allocation at crates/engine/src/wasm/process.rs:340, which runs with in_template_invocation == true:

Buggy_main → tari_engine (allowed) → handle → env_mut.alloc → template tari_alloc → tari_engine (still allowed) → handle → …

max_call_depth: 10 does not apply — it is the same call frame. max_logs caps EmitLog only; GenerateUniqueId and CallerContextInvoke have no count cap and always succeed. Metering will not get there first: 100M points against a ~100-point tari_alloc body is millions of rounds, and the native stack blows at a few thousand. A stack overflow is a SIGSEGV, not a catchable trap — so this aborts a consensus node from a template anyone can publish.

test_engine_call_in_tari_alloc does not reach it because the CallInfo alloc fails first. A tari_alloc that only calls the engine once a static counter passes 1 gets past that and into the loop.

The machinery added in fbd9dba closes it — bracket the response allocation the same way:

env_mut.exit_template_invocation();
let ptr = env_mut.alloc(&mut store, len as u32);
env_mut.enter_template_invocation();
take_refused_engine_call(env_mut)?;
let ptr = ptr?;

That also makes the rule uniform and matches what the PR title already claims: no engine call from tari_alloc/tari_free, ever. It is pre-existing, so a separate PR is defensible — but it is a live remote DoS and the fix belongs next to this one.

2. 6e29d17 introduces &mut aliasing across the WASM boundary

alloc_checked (crates/engine/src/wasm/process.rs:135-141) holds env: &mut WasmEnv<Runtime> from data_and_store_mut() across env.alloc(&mut store, len), which re-enters WASM. If the template's tari_alloc calls tari_engine, wasmer hands tari_engine_entrypoint a second &mut to that same WasmEnv, and it writes refused_engine_call through it — which the outer, still-live &mut then reads. Not hypothetical: it is the exact path test_engine_call_in_tari_alloc exercises. Same shape in free_checked and with_alloc_and_mem_writer.

Before this commit the host side used a separate WasmEnv object with Arc<Mutex<_>> fields, so the two &muts never pointed at the same allocation — the interior mutability was load-bearing, not just lock ceremony. data_and_store_mut obtains its &mut T through a raw-pointer split so this compiles, and Runtime already leans on the same trick by design. But it is a Stacked Borrows violation on plain, now non-atomic fields, and the kind of thing an optimiser is entitled to break later.

Cheap fix: do not hold the borrow across the call. TypedFunction is Clone, so pull it out, drop the borrow, call, then re-borrow to drain:

let alloc_fn = self.fn_env.as_ref(store).mem_alloc_func()?.clone();
let result = alloc_fn.call(store, len);
take_refused_engine_call(self.fn_env.as_mut(store))?;

handle has the same shape at line 340 and predates this commit — the bracketing in finding 1 wants the same treatment.

Minor

• The Err arm of invoke checks take_last_engine_error and take_last_panic_message but not take_refused_engine_call. Harmless today — every refusal is drained by the alloc or free that caused it — but if the guard extends to handle, a refusal followed by a template trap would be silently dropped and the trap reported instead. Worth draining there pre-emptively.
• enter_template_invocation/exit_template_invocation are manually paired with no ? between them, which is correct but fragile to future edits. A short comment noting that nothing may return early between them would help.

Reviewed b6e23d2 and 5f5bff6, plus the rebase. LGTM.

b6e23d2 closes the aliasing on both host-driven paths: the exported function is cloned out, the borrow dropped, the call made, then a fresh borrow drains the refusal. Refusal still takes precedence over the call's own error and over MemoryAllocationFailed, and moving the null check into alloc_checked is behaviour-preserving (WasmPtr::is_null() is offset == 0). The two remaining split borrows only touch linear memory and never re-enter WASM.

Rebase — compared each rewritten commit's patch against its original: identical modulo hunk offsets, except the one real conflict in 078a94cc where upstream had swapped wasm_point_allowance() for compute_allowance() -> ComputeAllowance and split the out-of-gas error. Resolution keeps upstream's shape; nothing lost.

5f5bff6 is a pure refactor. metering_allowance reproduces the extracted block exactly — same reads, same min chain, same binding_allowance filter; the reordering is all pure reads off one borrow, and get_remaining_points still runs first. consumed is correctly carried rather than re-read, since record_wasm_execution mutates the total before the Err arm reads it. match allowance_remaining doesn't move (_ never binds, u64 is Copy), so the later filter/map still sees it. The env/env_mut/env_and_store rename is applied consistently, and alloc_checked/free_checked keep the borrow released.

Nit: metering_allowance takes a concrete &mut Store while its siblings are generic over S: AsStoreMut. Only one caller, so it does not matter.

Two things before merge, neither about the code:

• The description still frames the fix around WasmEnv::invocation_meter being Some only between begin_metered_invocation and end_metered_invocation. b1ca1e6 deliberately replaced that with an explicit window. Worth updating, since it becomes the squashed commit message.
• Branch is BEHIND again, and test (3/3) / integration test are still running.

The remaining handle recursion and its aliasing are tracked in #2442 and correctly out of scope here.